Pex exits with correct code when using entrypoint #605
Conversation
There was a problem hiding this comment.
For the CI failure, this keeps the spirit of the existing failing test:
diff --git a/tests/test_integration.py b/tests/test_integration.py
index 36bee7a..aa2362e 100644
--- a/tests/test_integration.py
+++ b/tests/test_integration.py
@@ -1022,10 +1022,12 @@ def pex_with_entrypoints(entry_point):
""")
my_app = dedent("""
- from setuptools.sandbox import run_setup
-
def do_something():
- return run_setup
+ try:
+ from setuptools.sandbox import run_setup
+ return 0
+ except:
+ return 1
if __name__ == '__main__':
do_something()And this is an exact parallel for setting up the test you lay out in the description:
https://github.com/pantsbuild/pex/blob/884b4596a3d2d8a862fea566d1a6d3344b56e538/tests/test_integration.py#L826-L837
tests/test_integration.py
Outdated
| from setuptools.sandbox import run_setup | ||
| return 0 | ||
| except: | ||
| return 1 | ||
|
|
||
| if __name__ == '__main__': | ||
| do_something() |
There was a problem hiding this comment.
You beat me to it. As you see fit: #606
The do_something() should probably be used to sys.exit.
Currently the exit code from an entrypoint is swallowed, so running pex always exits 0. Tested with: ``` tox -e py27-package dist/pex27 -e pytest:main pytest -D /path/to/failing/test/dir ``` before this PR, this exits 0, after it exits 1. I'm not sure how to properly integration test this, as it is in the outer-most wrapper layer of the pex binary, so looks like it's bypassed by the way we tend to test things?
illicitonion
force-pushed
the
dwagnerhall/exit_from_entry_point
branch
from
b72e767 to
4bbfade
Compare
tests/test_integration.py
Outdated
| tester_path = os.path.join(output_dir, 'tester.py') | ||
| results = run_pex_command(['pytest==3.9.1', | ||
| '-e', 'pytest:main', | ||
| '-D', output_dir, |
There was a problem hiding this comment.
Don't embed the source to test in the pex, instead maybe just: subprocess.call([pex_path, os.path.join(output_dir, 'tester.py')]).
| @@ -854,7 +854,7 @@ def test_fail(): | |||
| '-o', pex_path]) | |||
| results.assert_success() | |||
|
|
|||
| assert subprocess.call([pex_path, tester_path]) == 1 | |||
| assert subprocess.call([pex_path, os.path.realpath(tester_path)]) == 1 | |||
| @@ -348,7 +346,9 @@ def execute(self): | |||
| self.patch_sys(pex_inherit_path) | |||
| working_set = self._activate() | |||
| self.patch_pkg_resources(working_set) | |||
| self._wrap_coverage(self._wrap_profiling, self._execute) | |||
| exit_code = self._wrap_coverage(self._wrap_profiling, self._execute) | |||
There was a problem hiding this comment.
This was never correct. Who knows is the entry point function even returns an int? The real problem was ever using the pytest:main entrypoint. We should have been using -c pytest or else -m pytest. Pex needs to provide some guidance in its --help and docs, but using an entrypoint function (foo.bar:baz) implies you know the function does its own sys.exits to indicate failure or else raises. If it just returns some value, whether an int or a string or a what have you, PEX really can't just presume and pas that to sys.exit.
There was a problem hiding this comment.
The right path may be to keep this behavior but actually document it. Pex could say "If you pass an entrypoint function and it returns anything, we'll assume that's an int that represents the exit code of the process that should be used."
It looks like sys.exit behavior is a bit whacky and gracefully supports the present use :/:
$ python -c 'import sys; sys.exit("Hello")'; echo $?
Hello
1
$ python -c 'import sys; sys.exit([])'; echo $?
[]
1
$ python -c 'import sys; sys.exit(["a"])'; echo $?
['a']
1
$ python -c 'import sys; sys.exit(True)'; echo $?
1
$ python -c 'import sys; sys.exit(False)'; echo $?
0
Currently the exit code from an entrypoint is swallowed, so running pex
always exits 0.
Tested with:
before this PR, this exits 0, after it exits 1.
I'm not sure how to properly integration test this, as it is in the
outer-most wrapper layer of the pex binary, so looks like it's bypassed
by the way we tend to test things?